fix(url_safety): exclude 198.18.0.0/15 benchmark range from SSRF private-IP blocking - #35436
Open
liuhao1024 wants to merge 1 commit into
Open
liuhao1024 wants to merge 1 commit into
liuhao1024 wants to merge 1 commit into
Conversation
…ate-IP blocking DNS-based content-filtering services (OpenDNS/Cisco Umbrella, CleanBrowsing, etc.) resolve blocked or sinkholed domains into the 198.18.0.0/15 benchmarking range. Python's ipaddress module marks this range as is_private, causing false-positive SSRF blocks on public sites like cdimage.ubuntu.com when accessed from filtered networks. The 198.18.0.0/15 range is not a real private/internal network used for SSRF targets — actual threats (RFC 1918, cloud metadata at 169.254.x.x) are handled by separate, stricter checks. Fixes NousResearch#35423
alt-glitch
added
type/bug
|
Related: #3777, #19992, #35423. The existing workaround is See also open PR #26064 which adds a more granular |
tonydwb
approved these changes
May 30, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (well-analyzed security fix)
Looks Good
- Correct root cause analysis: 198.18.0.0/15 is marked
is_privateby Python'sipaddressmodule, but it's the IANA-specified Benchmarking range (RFC 2544) — not an actual private network. DNS-based content filters (OpenDNS/Cisco Umbrella, CleanBrowsing) resolve sinkholed domains here, causing false-positive SSRF blocks on legitimate public sites. - Minimal blast radius: Only 198.18.0.0/15 is excluded. RFC 1918 (10.x, 172.16.x, 192.168.x), link-local (169.254.x.x), CGNAT (100.64.0.0/10), multicast, loopback all remain blocked.
- Both IPv4 and IPv4-mapped IPv6 paths handled: The early return in the IPv6 path (
if embedded_ip in _BENCHMARK_RANGE: return False) and the check before theis_privatereturn in the standard path both correctly exempt the range. - Cloud metadata remains blocked:
_ALWAYS_BLOCKED_IPSand_ALWAYS_BLOCKED_NETWORKS(169.254.169.254, etc.) are checked before_is_blocked_ipinis_safe_url, so this change cannot bypass cloud metadata protection. - Test coverage is thorough:
test_benchmark_range_allowed_for_any_host— general benchmark IP now allowedtest_cdimage_ubuntu_allowed_on_filtered_network— real-world regression testtest_qq_multimedia_subdomain_also_allowed_via_benchmark_range— verifies previous allowlist exception is now subsumedtest_qq_multimedia_http_allowed_via_benchmark_range— HTTP (previously blocked) now allowed- Updated
test_blocked_ipsparametrize: removed 198.18.0.23 from blocked list - Updated
test_allowed_ipsparametrize: added 198.18.0.23, 198.19.255.255 - Updated
test_ipv4_mapped_allowed_ips: added::ffff:198.18.0.65
Comments on the change to QQ hostname tests
- Previously
sub.multimedia.nt.qq.com.cnand HTTP-only QQ URLs were blocked (enforcing narrow allowlist). Now they're allowed because any host resolving to the benchmark range passes. This is a behavioral relaxation but it's the correct tradeoff — the benchmark range is not a real private network, so there's no SSRF risk. The QQ allowlist comment was also updated to clarify it now only covers hosts resolving to other private ranges.
No Security Concerns
- The 198.18.0.0/15 range cannot be used for SSRF attacks (cloud metadata is on 169.254.169.254, internal services are on RFC 1918). This is a genuine false-positive fix.
_TRUSTED_PRIVATE_IP_HOSTSstill provides protection for any QQ-like hosts that resolve to real private ranges.
Reviewed by Hermes Agent
tonydwb
approved these changes
May 30, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Overall
Clean, well-scoped fix for a real-world SSRF false-positive issue. The benchmark range (198.18.0.0/15) is used by DNS-based content-filtering services (OpenDNS, CleanBrowsing) to sinkhole blocked domains — blocking it creates false positives on filtered networks without meaningful SSRF protection.
What's Good
- Correct range choice: 198.18.0.0/15 is the documented IANA benchmark range, not a real private network
- Proper reasoning: RFC 1918, cloud metadata (169.254.x.x), and CGNAT (100.64.0.0/10) remain fully blocked — actual SSRF targets are protected
- Dual-path coverage: Both IPv4 and IPv4-mapped IPv6 paths are covered
- Comprehensive tests: Regression test for cdimage.ubuntu.com, updated all existing benchmark IP tests, parametrized allowed-IP lists updated, existing QQ hostname tests properly reflect new behavior
- Good docstrings: Explain why each test expectation changed, making reviewer's job easy
- Clean blast radius: LOW — narrow change to IP classification logic only
No Issues Found
- No edge cases missed (the range is a single /15, no sub-ranges to worry about)
- No security regression — RFC 1918, loopback, link-local, multicast, CGNAT all unchanged
- Tests were correctly updated to reflect the new behavioral expectations
Reviewed by Hermes Agent
13 tasks
teknium1
reviewed
Jul 13, 2026
teknium1
left a comment
teknium1
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for documenting the RFC 2544 false-positive and covering both ordinary and IPv4-mapped address classification.
Problems
- The proposed default exemption removes an intentional current-main boundary. Main allows 198.18.0.0/15 only for one exact HTTPS QQ host (
tools/url_safety.py:179-184,379-381); tests deliberately keep generic hosts, subdomains, and HTTP blocked (tests/tools/test_url_safety.py:247-269). The PR changes all three cases to allowed by default.
Suggested changes
- Preserve the default boundary and, if maintainers want broader support, re-scope it as an explicit
config.yamlopt-in. The member discussion already points to the narrowerallow_benchmark_ipsdirection in #26064. - Add an end-to-end
is_safe_url()test for an IPv4-mapped benchmark address under any new scoped option, while retaining coverage that RFC 1918, CGNAT, and link-local addresses remain blocked.
Automated hermes-sweeper review.
| # (OpenDNS/Cisco Umbrella, CleanBrowsing, etc.) resolve blocked or | ||
| # sinkholed domains into this range. Public sites like cdimage.ubuntu.com | ||
| # can legitimately resolve here on filtered networks. Blocking the entire | ||
| # range produces false positives without meaningful SSRF protection — the |
Collaborator
There was a problem hiding this comment.
This default-wide bypass changes the deliberate current policy from one exact HTTPS host to every hostname and scheme resolving into 198.18.0.0/15. Please preserve the default SSRF boundary and make broader allowance an explicit config.yaml opt-in if maintainers choose that direction.
teknium1
added
sweeper:risk-security-boundary
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Excludes the 198.18.0.0/15 benchmark/DNS-filtering range from SSRF private-IP blocking, fixing false-positive blocks on public sites like
cdimage.ubuntu.comwhen accessed from networks using DNS-based content-filtering services.Related Issue
Fixes #35423
Type of Change
Changes Made
tools/url_safety.py: Added_BENCHMARK_RANGE(198.18.0.0/15) and excluded it from_is_blocked_ip()for both IPv4 and IPv4-mapped IPv6 addresses. Updated_TRUSTED_PRIVATE_IP_HOSTScomment to note the new range exclusion.tests/tools/test_url_safety.py: Updated existing tests and added regression tests for benchmark range IP allowance, includingcdimage.ubuntu.comspecific test case and IPv4-mapped IPv6 benchmark range test.How to Test
pytest tests/tools/test_url_safety.py -v— all 115 tests should passcdimage.ubuntu.com(resolves to 198.18.0.65 on DNS-filtered networks) is no longer blocked byis_safe_url()Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/ACode Intelligence
tools/url_safety.py:_is_blocked_ip(called byis_safe_url,is_always_blocked_url; callers:web_tools.py,browser_tool.py,vision_tools.py)_TRUSTED_PRIVATE_IP_HOSTS(QQ media per-host exception for same range),_CGNAT_NETWORK(similar range-exclusion pattern)